[RFR] Responsive chart #249

zyhou · 2018-07-05T08:04:07Z

Make responsive great again.

Todo review

Can't we do it in the resize event instead? We don't need to recompute it everytime if window size is kept the same.

fixes #245

PS: Test in other PR with cypress

jpetitcolas

We agreed to implement E2E tests to ensure resizing works, as JSDOM doesn't seem to support clientWidth property.

jpetitcolas · 2018-07-05T13:02:00Z

src/index.js

@@ -73,8 +77,24 @@ export default ({ d3 = window.d3, ...customConfiguration }) => {
            .call(draw(config, xScale));
    };

+    const chart = selection => {
+        const root = selection.selectAll('svg').data(selection.data());


Move these two lines in createChart

jpetitcolas · 2018-07-05T13:02:31Z

src/index.js

+        window.removeEventListener('resize', callback, true);
+    };
+
+    const createChart = (root, selection) => {


jpetitcolas · 2018-07-05T13:05:48Z

docs/configuration.md

@@ -326,3 +326,9 @@ This parameter configures the minimum zoom level available. Set it to a not-null
 _Default: Infinity_

 This parameter configures the maximum zoom level available. Set it to a lower value to prevent your users from zooming in too deeply.
+
+### removeOnResize


I would remove this configuration parameter. Instead, I would expose a destroy method taking a callback as argument. Something like:

destroy: callback => { global.removeEventListener('resize', resizeListener); callback(); }

jpetitcolas · 2018-07-05T13:07:09Z

src/index.js

@@ -12,7 +12,15 @@ import { withinRange } from './withinRange';

 // do not export anything else here to keep window.eventDrops as a function
 export default ({ d3 = window.d3, ...customConfiguration }) => {
-    const chart = selection => {
+    const onResize = callback => {


No need for these functions. Just keep a reference to the event listener to be able to remove it in the destroy method.

jpetitcolas · 2018-07-09T07:06:51Z

src/index.js

-        chart._removeOnResize = () => removeOnResize(updateChart);
+        window.addEventListener('resize', updateChart, true);
+
+        chart._removeOnResize = () =>


Do we really need a function for this single line? :)

jpetitcolas · 2018-07-09T07:08:49Z

docs/configuration.md


 _Default: () => {}_

-Function to be executed when you want to remove [`resize`](https://developer.mozilla.org/en-US/docs/Web/Events/resize) event.
+Function to be executed when you want to destroy the chart. By default, it remove `resize` event and execute the callback.


s/Function to be executed when you want to destroy the chart. By default, it remove resize event and execute the callback./Execute this function before to removing the chart from DOM. It prevents some memory leaks due to event listeners./

jpetitcolas · 2018-07-09T07:09:47Z

docs/configuration.md

@@ -327,8 +327,8 @@ _Default: Infinity_

 This parameter configures the maximum zoom level available. Set it to a lower value to prevent your users from zooming in too deeply.

-### removeOnResize
+### destroy


I would move it to: https://github.com/marmelab/EventDrops/blob/master/README.md#interface

jpetitcolas · 2018-07-09T12:10:44Z

README.md

-* **draw(config, scale)** redraw chart using given configuration and `d3.scaleTime` scale
+*   **scale()** provides the horizontal scale, allowing you to retrieve bounding dates thanks to `.scale().domain()`,
+*   **filteredData()** returns an object with both `data` and `fullData` keys containing respectively bounds filtered data and full dataset.
+*   **draw(config, scale)** redraw chart using given configuration and `d3.scaleTime` scale


s/redraw/redraws/

As requested by jpetitcolas at #249 (comment)

jpetitcolas · 2018-07-09T12:11:52Z

README.md

+*   **scale()** provides the horizontal scale, allowing you to retrieve bounding dates thanks to `.scale().domain()`,
+*   **filteredData()** returns an object with both `data` and `fullData` keys containing respectively bounds filtered data and full dataset.
+*   **draw(config, scale)** redraw chart using given configuration and `d3.scaleTime` scale
+*   **destroy** execute this function before to removing the chart from DOM. It prevents some memory leaks due to event listeners.


s/execute this function before to removing the chart from DOM/should be executed whenever you remove the chart from DOM/

jpetitcolas · 2018-07-09T12:13:10Z

docs/configuration.md

+\_Default:
+
+```js
+const chart = eventDrops({


That's not the default value. Display default value only here.

jpetitcolas · 2018-07-09T12:15:30Z

docs/configuration.md

+    },
+});
+
+This parameter configures the number of ticks display in the header


I don't understand this sentence. I propose:

When reducing chart width, we need to display less labels on the horizontal axis to keep a readable chart. This parameter aims to solve the issue. Hence, on smallest devices, it displays only 3 labels by default at the same time.

And add a list of breakpoint resolutions to know what small means.

jpetitcolas · 2018-07-09T12:16:43Z

src/axis.js

+import breakpoints from './breakpoints';
+
+export const getBreakpointLabel = point =>
+    Object.keys(breakpoints).reduce((accumulator, label) => {


Why not just using a for loop? It would prevent browsing every breakpoints for smallest devices.

jpetitcolas · 2018-07-09T12:17:21Z

src/axis.js

@@ -44,6 +59,11 @@ export default (d3, config, xScale) => {
            .axisTop(xScale)
            .tickFormat(d => tickFormat(d, formats, d3));

+        const breakpoint = getBreakpointLabel(window.innerWidth) || 'extra';


Can't we do it in the resize event instead? We don't need to recompute it everytime if window size is kept the same.

jpetitcolas · 2018-07-09T12:17:38Z

src/axis.js

@@ -44,6 +59,11 @@ export default (d3, config, xScale) => {
            .axisTop(xScale)
            .tickFormat(d => tickFormat(d, formats, d3));

+        const breakpoint = getBreakpointLabel(window.innerWidth) || 'extra';
+        if (numberDisplayedTicks) {


Do we need an ifhere, with the default value?

jpetitcolas · 2018-07-09T12:18:03Z

src/axis.spec.js

@@ -120,8 +120,37 @@ describe('Axis', () => {
        expect(timeFormatDefaultLocaleSpy).toHaveBeenCalledWith(defaultLocale);
    });

+    it('should display tick using configuration locale', () => {


Not sure why the locale intervenes here. :)

jpetitcolas · 2018-07-09T12:18:57Z

src/axis.spec.js

+            numberDisplayedTicks: { extra: 9 },
+        };
+
+        axis(d3, config, defaultScale)(selection);


Use a test function to not repeat yourself and focus more on what you are testing.

jpetitcolas · 2018-07-09T12:19:15Z

src/breakpoints.js

@@ -0,0 +1,6 @@
+export default {
+    small: 576,


Shouldn't it be in configuration?

jpetitcolas · 2018-07-12T07:29:42Z

src/index.js

-        const root = selection.selectAll('svg').data(selection.data());
-
-        root.exit().remove();
+        chart._breakpoint = getBreakpointLabel(breakpoints, window.innerWidth);


Use global instead of window

Rename to currentBreakpointLabel

Add documentation and a test to ensure we can sill access this attribute.

jpetitcolas · 2018-07-12T07:31:25Z

src/axis.js

@@ -1,5 +1,15 @@
 import { timeFormat } from 'd3-time-format';

+export const getBreakpointLabel = (breakpoints, point) => {


Move it to index.js as you use it here. But as rollup requires a single export in this file, we probably need an extra breakpoint.js file.

jpetitcolas · 2018-07-12T15:20:29Z

README.md

@@ -84,9 +84,11 @@ You can either use D3 as a specific import (specifying it in first argument of `

 In addition to this configuration object, it also exposes some public methods allowing you to customize your application based on filtered data:


s/methods/members/

jpetitcolas · 2018-07-12T15:21:11Z

README.md

+*   **filteredData()** returns an object with both `data` and `fullData` keys containing respectively bounds filtered data and full dataset.
+*   **draw(config, scale)** redraws chart using given configuration and `d3.scaleTime` scale
+*   **destroy** execute this function before to removing the chart from DOM. It prevents some memory leaks due to event listeners.
+*   **currentBreakpointLabel** returns a label of current breakpoint, [list of breakpoints](./docs/configuration.md#breakpoints).


s/returns a label of current breakpoint, [list of breakpoints]/returns current breakpoint (for instance small) among a [list of breakpoints]/

jpetitcolas · 2018-07-12T15:22:30Z

README.md

+*   **scale()** provides the horizontal scale, allowing you to retrieve bounding dates thanks to `.scale().domain()`,
+*   **filteredData()** returns an object with both `data` and `fullData` keys containing respectively bounds filtered data and full dataset.
+*   **draw(config, scale)** redraws chart using given configuration and `d3.scaleTime` scale
+*   **destroy** execute this function before to removing the chart from DOM. It prevents some memory leaks due to event listeners.


s/destroy/destroy()/

As requested by jpetitcolas at #249 (comment)

…s]/returns current breakpoint (for instance `small`) among a [list of breakpoints]/ As requested by jpetitcolas at #249 (comment)

As requested by jpetitcolas at #249 (comment)

As requested by jpetitcolas at marmelab#249 (comment)

…s]/returns current breakpoint (for instance `small`) among a [list of breakpoints]/ As requested by jpetitcolas at marmelab#249 (comment)

As requested by jpetitcolas at marmelab#249 (comment)

zyhou added 3 commits July 5, 2018 09:56

responsive chart resize event

d98f6d9

add removeEventListener event

3afe593

remove ie9 condition + add doc removeOnResize function

5935c75

zyhou changed the title ~~[WIP] Responsive chart~~ [RFR] Responsive chart Jul 5, 2018

jpetitcolas suggested changes Jul 5, 2018

View reviewed changes

zyhou added 5 commits July 5, 2018 15:26

apply review

953e672

add breakpoint and display count tick config

6a437fd

add doc

d5f3d5f

add test getBreakpointLabel

34a2036

fix test

569054a

zyhou changed the title ~~[RFR] Responsive chart~~ [WIP] Responsive chart Jul 5, 2018

add test display tick

d04a819

zyhou changed the title ~~[WIP] Responsive chart~~ [RFR] Responsive chart Jul 6, 2018

jpetitcolas suggested changes Jul 9, 2018

View reviewed changes

zyhou added 2 commits July 9, 2018 13:41

review doc

ed82c73

review initChart

9b7dee7

jpetitcolas reviewed Jul 9, 2018

View reviewed changes

Typo fix s/redraw/redraws/

de5fb75

As requested by jpetitcolas at #249 (comment)

jpetitcolas reviewed Jul 9, 2018

View reviewed changes

jpetitcolas suggested changes Jul 9, 2018

View reviewed changes

zyhou added 3 commits July 9, 2018 17:51

add breakpoint in doc

36ffb89

fix generate ticks + test

871acb9

remove duplicate test

fdbf768

zyhou changed the title ~~[RFR] Responsive chart~~ [WIP] Responsive chart Jul 9, 2018

zyhou added 2 commits July 10, 2018 17:46

add breakpoints in config

91f2365

calcul breakpoint on redraw

eacc059

zyhou changed the title ~~[WIP] Responsive chart~~ [RFR] Responsive chart Jul 12, 2018

jpetitcolas suggested changes Jul 12, 2018

View reviewed changes

review about breakpointLabel

2be4abb

jpetitcolas suggested changes Jul 12, 2018

View reviewed changes

sedy-bot added 3 commits July 12, 2018 15:22

Typo fix s/methods/members/

fd56c95

As requested by jpetitcolas at #249 (comment)

Typo fix s/returns a label of current breakpoint, [list of breakpoint…

1ff0a37

…s]/returns current breakpoint (for instance `small`) among a [list of breakpoints]/ As requested by jpetitcolas at #249 (comment)

Typo fix s/destroy/destroy()/

f18f262

As requested by jpetitcolas at #249 (comment)

jpetitcolas merged commit b5344db into master Jul 12, 2018

jpetitcolas deleted the responsive-chart branch July 12, 2018 15:23

jaredkirkley mentioned this pull request May 15, 2019

Zoom improvements #269

Open

papigers pushed a commit to Stratoscale/EventDrops that referenced this pull request Dec 23, 2020

Typo fix s/redraw/redraws/

598471e

As requested by jpetitcolas at marmelab#249 (comment)

papigers pushed a commit to Stratoscale/EventDrops that referenced this pull request Dec 23, 2020

Typo fix s/methods/members/

383b6b1

As requested by jpetitcolas at marmelab#249 (comment)

papigers pushed a commit to Stratoscale/EventDrops that referenced this pull request Dec 23, 2020

Typo fix s/destroy/destroy()/

a0e03eb

As requested by jpetitcolas at marmelab#249 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFR] Responsive chart #249

[RFR] Responsive chart #249

zyhou commented Jul 5, 2018 •

edited

Loading

jpetitcolas left a comment

jpetitcolas Jul 5, 2018

jpetitcolas Jul 5, 2018

jpetitcolas Jul 5, 2018

jpetitcolas Jul 5, 2018

jpetitcolas Jul 9, 2018

jpetitcolas Jul 9, 2018

jpetitcolas Jul 9, 2018

jpetitcolas Jul 9, 2018

jpetitcolas Jul 9, 2018

jpetitcolas Jul 9, 2018

jpetitcolas Jul 12, 2018

jpetitcolas Jul 9, 2018

jpetitcolas Jul 9, 2018

jpetitcolas Jul 9, 2018

jpetitcolas Jul 9, 2018

jpetitcolas Jul 9, 2018

jpetitcolas Jul 9, 2018

jpetitcolas Jul 9, 2018

jpetitcolas Jul 12, 2018

jpetitcolas Jul 12, 2018

jpetitcolas Jul 12, 2018

jpetitcolas Jul 12, 2018

jpetitcolas Jul 12, 2018

jpetitcolas Jul 12, 2018

jpetitcolas Jul 12, 2018

		@@ -1,5 +1,15 @@
		import { timeFormat } from 'd3-time-format';

		export const getBreakpointLabel = (breakpoints, point) => {

		@@ -84,9 +84,11 @@ You can either use D3 as a specific import (specifying it in first argument of `

		In addition to this configuration object, it also exposes some public methods allowing you to customize your application based on filtered data:

[RFR] Responsive chart #249

[RFR] Responsive chart #249

Conversation

zyhou commented Jul 5, 2018 • edited Loading

Todo review

jpetitcolas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyhou commented Jul 5, 2018 •

edited

Loading